Fail fast in the simulator on surfaces usage that stalls a device - #5491
Merged
Conversation
Surfaces publishing is cheap in the simulator and expensive on hardware: the same call that is a Java2D encode plus a local file write here writes into a shared app-group container, hands the payload to WidgetKit or ActivityKit over IPC and, for any image that is not already an EncodedImage, blocks the caller on the platform UI thread while the pixels are read back off the GPU (IOSNative.m createImageFile does a Metal flush and a dispatch_sync onto main). An app that publishes on the EDT therefore looks fine in the simulator and freezes on a phone, which is the worst possible place to find out. Discussion #5490 is exactly this: every call funnelled onto the EDT through callSerially, an icon handed over as a live Image, and a widget kind that was never registered. Add SurfaceDiagnostics, active only when Display.isSimulator() (so a shipped build pays nothing) and overridable with the new Surfaces.setDiagnosticsEnabled(Boolean). Conditions that are certain to misbehave on a device throw IllegalStateException naming the fix; the rest log once: - throws when a non-EncodedImage is rasterized on the EDT, checked in SurfaceSerializer.encode so the stack lands on the app's SurfaceImage - throws when publish() targets a kind that was never registered, and names the kinds that are registered so a typo is obvious - warns once when publish/start/update/end runs on the EDT - warns when one kind or activity is republished past the platform's reload budget, pointing at SurfaceDynamicText and future timeline entries as the way to avoid it - warns when an inert LiveActivity handle is used, since update/end are silent no-ops and that is why a refused start goes unnoticed The image check caught the same bug in our own reference code: SurfacesSample generated a mutable avatar and published it from EDT button handlers, with a comment conceding a real app would ship an EncodedImage. It now caches one, generating the pixels on the caller's thread and encoding inside invokeAndBlock; the developer-guide snippet's courierAvatar field changes to EncodedImage for the same reason. Surfaces.publish claimed "no step blocks on the EDT or the platform UI thread", which was never true of the rasterizing encode; that clause is corrected and LiveActivity.start gains the threading section it lacked. SurfaceTest 28/28, full core-unittests module 4286/4286. Verified in a real simulator run both ways: the fixed sample publishes with only the EDT warning, a deliberately-broken copy throws with the stack pointing at its SurfaceImage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec3af8a708
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Adds simulator-only “fail fast” diagnostics to the Surfaces API to catch usage patterns that appear fine in the JavaSE simulator but can stall or silently no-op on real devices (notably iOS), and updates samples/docs/tests accordingly.
Changes:
- Introduces
SurfaceDiagnosticsand wires it intoSurfaces.publish(),LiveActivity.start/update/end(), andSurfaceSerializer.encode()to throw on known-bad patterns and warn once on likely-bad patterns (simulator-only unless explicitly overridden). - Adds unit tests covering diagnostic enablement/inertness and key failure/warn behaviors.
- Updates the Surfaces sample and developer-guide snippets to use cached
EncodedImagepayloads to avoid rasterizing images during publish.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Samples/samples/SurfacesSample/SurfacesSample.java | Caches the courier avatar as an EncodedImage to avoid publish-time rasterization on EDT. |
| maven/core-unittests/src/test/java/com/codename1/surfaces/SurfaceTest.java | Adds tests for simulator diagnostics behavior (throw/warn/inert). |
| docs/demos/common/src/main/java/com/codenameone/developerguide/surfaces/SurfacesSnippets.java | Updates snippet to use EncodedImage and adds standard header. |
| CodenameOne/src/com/codename1/surfaces/SurfaceSerializer.java | Calls diagnostics hook before rasterizing non-EncodedImage payloads. |
| CodenameOne/src/com/codename1/surfaces/Surfaces.java | Adds diagnostics enable/disable API and invokes diagnostics during publish/reset. |
| CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java | New simulator-only diagnostics implementation (throws + one-time warnings). |
| CodenameOne/src/com/codename1/surfaces/LiveActivity.java | Adds threading docs and diagnostics hooks for EDT usage, rate limiting, and inert handles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Two findings from the PR review bots, both correct. registeredKinds was a plain ArrayList that registerWidgetKind() mutated while the new isKindRegistered() (and registerWidgetKind itself) walked it, so a registration racing a publish could throw ConcurrentModificationException. The API is explicitly callable from any thread, so guard every touch of the list with its own monitor and let readers copy out rather than iterate live. This also closes the same hazard that already existed in registerWidgetKind before this branch. The EDT image diagnostic suggested EncodedImage.createFromImage(img, false) as the remedy without saying where to run it. Following that advice at the same call site performs the identical ImageIO.save and pays the very stall the check exists to prevent -- and then hides it, because the serializer afterwards sees an EncodedImage. The message now says to convert ONCE, off the EDT, and cache the result, which is what the updated sample does. Adds a regression test that pads the registry and looks the target up last so the lookup genuinely overlaps the writer. Being a race it reproduces the unsynchronized failure about one run in three; it never false-fails with the locking in place (5 consecutive clean runs). SurfaceTest 29/29. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daf7051afa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+143
to
+144
| if (!enabled() || Surfaces.isKindRegistered(kindId)) { | ||
| return; |
There was a problem hiding this comment.
When a reader runs the repository's SurfacesSnippets examples, registerKinds() registers only delivery_status, while publishClockTimeline() publishes analog_clock; because diagnostics are enabled by default in the simulator, this new check throws before the documented clock timeline can be published. Add analog_clock to the guide's runtime registrations and matching manifest example so the documented flow remains runnable.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java:62
overrideandedtForTestsare written insetEnabled()/setEdtForTests()and read inenabled()/isEdt()without any synchronization. Since surfaces APIs (and these diagnostics) are callable from any thread, this is a real Java memory-visibility race (other threads may see stale values). Mark these fieldsvolatile(or useAtomicReference<Boolean>) to make toggling diagnostics/test seams reliably visible across threads.
/// null = follow the platform (simulator on, everything else off), non-null = forced.
private static Boolean override;
/// null = ask the real Display, non-null = forced (tests only, see [#setEdtForTests(Boolean)]).
private static Boolean edtForTests;
CodenameOne/src/com/codename1/surfaces/Surfaces.java:111
- Removing from
registeredKindsinside an enhanced for-loop is brittle (it relies on breaking immediately to avoid iterator state checks). Using an explicitIteratorandIterator.remove()avoids any risk ofConcurrentModificationExceptionif this loop is refactored later, and makes the intent clearer.
for (WidgetKind k : registeredKinds) {
if (k.getId().equals(kind.getId())) {
registeredKinds.remove(k);
break;
}
CodenameOne/src/com/codename1/surfaces/Surfaces.java:193
publish()doesn’t validatekindIdortimeline. Iftimelineis null,SurfaceSerializer.serializeTimeline()will throw aNullPointerException(dereferencingtimeline) rather than a clear argument error. Adding an explicit guard improves API behavior and makes failures easier to diagnose (especially now that diagnostics run before serialization).
public static void publish(String kindId, WidgetTimeline timeline) {
SurfaceDiagnostics.requireRegisteredKind(kindId);
SurfaceDiagnostics.offEdtPreferred("Surfaces.publish");
SurfaceDiagnostics.noteRepublish("kind:" + kindId, "widget kind \"" + kindId + "\"");
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prompted by discussion #5490: an app that launches a Live Activity works in the simulator and freezes on an iPhone 13. The reported code funnels every surfaces call onto the EDT with
callSerially, handsSurfaceImagea livecom.codename1.ui.Image, and publishes a widget timeline for a kind it never registered. None of that is visible in the simulator.Why the simulator hides it
Publishing here is a Java2D encode plus a local file write. On a device the same call writes into a shared app-group container, hands the payload to WidgetKit or ActivityKit over IPC, and -- for any image that is not already an
EncodedImage-- blocks the caller on the platform UI thread while the pixels come back off the GPU (IOSNative.mcreateImageFiledrains the render queue anddispatch_syncs onto main). Doing that on the EDT stalls the UI on hardware and costs nothing here.What this adds
SurfaceDiagnostics, active only whenDisplay.isSimulator()so a shipped build pays nothing, overridable via the newSurfaces.setDiagnosticsEnabled(Boolean). Conditions certain to misbehave on a device throwIllegalStateExceptionnaming the fix; the rest log once.Throws:
EncodedImagerasterized on the EDT -- checked insideSurfaceSerializer.encodeso the stack trace lands on the app's ownSurfaceImagepublish()targeting a kind that was never registered, listing the kinds that are registered so a typo is obviousWarns once:
publish/start/update/endcalled on the EDTSurfaceDynamicTextand future timeline entries as the way outLiveActivityhandle being used, sinceupdateandendare silent no-ops and that is precisely why a refusedstartgoes unnoticedIt caught the same bug in our own reference code
SurfacesSamplegenerated a mutable avatar and published it from EDT button handlers, with a comment conceding a real app would ship anEncodedImage. It now caches one, generating the pixels on the caller's thread and encoding insideinvokeAndBlock. The developer-guide snippet'scourierAvatarfield changes toEncodedImagefor the same reason.Docs
Surfaces.publishclaimed "no step blocks on the EDT or the platform UI thread", which was never true of the rasterizing encode -- that clause is corrected, andLiveActivity.startgains the threading section it lacked.SurfacesSnippets.javapicks up the standard header because the copyright gate pulls touched files into scope.Verification
SurfaceTest28/28 (10 new), fullcore-unittestsmodule 4286/4286SurfaceImageThe throw-on-EDT case is unit-tested through
SurfaceDiagnosticsdirectly rather than throughSurfaceSerializer, because constructing a non-EncodedImageImageneeds a platformDisplaythat suite deliberately does not have -- the end-to-end proof is the simulator run.spotbugsandjavadocwere not run locally (they fail on JDK 25 for pre-existing reasons), so CI is the first check on those.🤖 Generated with Claude Code